Issue 29680696: [$csp4 adblockpluschrome] Issue 5241 - Add support for Content Security Policy filters (Closed)
|
Created:
Jan. 26, 2018, 6:05 p.m. by kzar Modified:
March 27, 2018, 12:05 p.m. Reviewers:
Sebastian Noack CC:
Manish Jethani, Thomas Greiner Visibility:
Public. |
DescriptionIssue 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 #
MessagesTotal messages: 27
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 potentially relevant for our ext/background.js code, I'm going to look into that next week.
https://codereview.adblockplus.org/29680696/diff/29680697/_locales/en_US/mess... File _locales/en_US/messages.json (right): https://codereview.adblockplus.org/29680696/diff/29680697/_locales/en_US/mess... _locales/en_US/messages.json:69: "message": "Invalid Content Security Policy" This message is not being displayed properly so far but I've run out of time to figure out why. Will sort that out next week.
Patch Set 2 : Rebased and tidied a few things up https://codereview.adblockplus.org/29680696/diff/29680697/_locales/en_US/mess... File _locales/en_US/messages.json (right): https://codereview.adblockplus.org/29680696/diff/29680697/_locales/en_US/mess... _locales/en_US/messages.json:69: "message": "Invalid Content Security Policy" On 2018/01/26 18:13:15, kzar wrote: > This message is not being displayed properly so far but I've run out of time to > figure out why. Will sort that out next week. Oh, it was an unrelated (and now fixed) bug #6417.
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 is only one line in the block? 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). Might this be the case when XHR (or fetch) is used from inside a ServiceWorker? https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode36 lib/csp.js:36: details.url != new URL(details.initiator).toString())) Why is it necessary to normalize the URL? Shouldn't all URLs already be normalized when reported through the extension API? https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode44 lib/csp.js:44: let specificOnly = false; Nit: This variable seems superfluous here. You can just inline the value false when calling matchesAny() here, and only define the variable below when you need to check for its value. https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode57 lib/csp.js:57: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) If we check whether the document is whitelisted, after matching the filter, wouldn't this cause wrong results in the devtools panel, i.e. showing an active filter while it doesn't apply due to whitelisting?
Patch Set 3 : Get devtools logging working (Adding Thomas to Cc since I mentioned him.) 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: } On 2018/03/06 19:43:24, Sebastian Noack wrote: > Nit: Don't we omit braces anymore if there is only one line in the block? The rule is to not omit the braces if the block (or the condition) spans multiple lines. I think otherwise it's left to the author's preferences regarding being consistent between if... else blocks, or adding them when not strictly required. I had push back from Thomas IIRC when mentioning these nits in codereviews and since more-or-less gave up. I think at the end of the day it doesn't matter too much. 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). On 2018/03/06 19:43:24, Sebastian Noack wrote: > Might this be the case when XHR (or fetch) is used from inside a ServiceWorker? I'm not sure what's causing this so far, I know the behaviour doesn't happen in Firefox. I filed a Chromium bug for this but had little help so far. https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode36 lib/csp.js:36: details.url != new URL(details.initiator).toString())) On 2018/03/06 19:43:24, Sebastian Noack wrote: > Why is it necessary to normalize the URL? Shouldn't all URLs already be > normalized when reported through the extension API? Well if you add this debugging code in the listener: let normalised = new URL(details.initiator).toString(); if (details.url == normalised && normalised != details.initiator) { console.log("url", details.url, "initiator", details.initiator, "normalised", normalised); } and then browse to https://regex101.com/, you'll see this logged: url https://regex101.com/ initiator https://regex101.com normalised https://regex101.com/ Perhaps there's a more efficient approach that would have the same result, I'm not sure. I figured if it could happen with trailing slashes perhaps it could happen in other ways too (e.g. punycode) and so I did it this way. https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode44 lib/csp.js:44: let specificOnly = false; On 2018/03/06 19:43:24, Sebastian Noack wrote: > Nit: This variable seems superfluous here. You can just inline the value false > when calling matchesAny() here, and only define the variable below when you need > to check for its value. Strictly you're right, but I added it early in an attempt to make our intent clearer with first checking for all matches and then later checking if specificOnly should apply. If you don't mind too much I'd prefer to keep it this way. https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode57 lib/csp.js:57: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) On 2018/03/06 19:43:24, Sebastian Noack wrote: > If we check whether the document is whitelisted, after matching the filter, > wouldn't this cause wrong results in the devtools panel, i.e. showing an active > filter while it doesn't apply due to whitelisting? Well your comment made me realise I so far didn't log these CSP matches to the devtools panel. I've fixed that now and it's working OK for me. https://codereview.adblockplus.org/29680696/diff/29716591/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29680696/diff/29716591/metadata.chrome#new... metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js (Incidental change required since I updated the adblockplusui dependency. Might not be necessary to include by the time this gets pushed.)
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). On 2018/03/07 15:07:01, kzar wrote: > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > Might this be the case when XHR (or fetch) is used from inside a > ServiceWorker? > > I'm not sure what's causing this so far, I know the behaviour doesn't happen in > Firefox. I filed a Chromium bug for this but had little help so far. Is there an example in which simply ignoring "xmlhttprequest" would give false negatives? My suspicion is that these requests are actual XHR/fetch requests, and that the tabId of -1 doesn't indicate a loading document, but that the origin is a ServiceWorker. https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode36 lib/csp.js:36: details.url != new URL(details.initiator).toString())) On 2018/03/07 15:07:01, kzar wrote: > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > Why is it necessary to normalize the URL? Shouldn't all URLs already be > > normalized when reported through the extension API? > > Well if you add this debugging code in the listener: > > let normalised = new URL(details.initiator).toString(); > if (details.url == normalised && normalised != details.initiator) > { > console.log("url", details.url, "initiator", details.initiator, > "normalised", normalised); > } > > and then browse to https://regex101.com/, you'll see this logged: > > url https://regex101.com/ initiator https://regex101.com normalised > https://regex101.com/ > > Perhaps there's a more efficient approach that would have the same result, I'm > not sure. I figured if it could happen with trailing slashes perhaps it could > happen in other ways too (e.g. punycode) and so I did it this way. FWIW, this seems like a Chrome bug to me. All other URLs reported through the API seem to be already normalized. https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode57 lib/csp.js:57: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) On 2018/03/07 15:07:01, kzar wrote: > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > If we check whether the document is whitelisted, after matching the filter, > > wouldn't this cause wrong results in the devtools panel, i.e. showing an > active > > filter while it doesn't apply due to whitelisting? > > Well your comment made me realise I so far didn't log these CSP matches to the > devtools panel. I've fixed that now and it's working OK for me. Forget about my original comment here. I mistakenly thought that filter matches are implicitly logged. But good thing, we now thought about logging to the devtools panel, at all. https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode66 lib/csp.js:66: hostname, cspMatch); This is redundant, checkWhitelisted() is already calling logWhitelistedDocument(). https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode72 lib/csp.js:72: Nit: I think it reads slightly better without a blank line here. Wouldn't insist though. https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode100 lib/csp.js:100: if (devtools) Is this check still necessary? IIRC, require('devtools') only returned null on Safari, where we don't include that module. Now, where we use WebPack it even gets automatically included as soon as any other module requires it.
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): https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode32 lib/csp.js:32: // the URL matches the initiator's URL (once normalised). On 2018/03/07 17:27:07, Sebastian Noack wrote: > On 2018/03/07 15:07:01, kzar wrote: > > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > > Might this be the case when XHR (or fetch) is used from inside a > > ServiceWorker? > > > > I'm not sure what's causing this so far, I know the behaviour doesn't happen > in > > Firefox. I filed a Chromium bug for this but had little help so far. > > Is there an example in which simply ignoring "xmlhttprequest" would give false > negatives? My suspicion is that these requests are actual XHR/fetch requests, > and that the tabId of -1 doesn't indicate a loading document, but that the > origin is a ServiceWorker. Yes, the website I noticed this happening was regex101.com. Add a $csp filter for that website and then try adjusting this listener to ignore xmlhttprequest events. https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode36 lib/csp.js:36: details.url != new URL(details.initiator).toString())) On 2018/03/07 17:27:07, Sebastian Noack wrote: > On 2018/03/07 15:07:01, kzar wrote: > > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > > Why is it necessary to normalize the URL? Shouldn't all URLs already be > > > normalized when reported through the extension API? > > > > Well if you add this debugging code in the listener: > > > > let normalised = new URL(details.initiator).toString(); > > if (details.url == normalised && normalised != details.initiator) > > { > > console.log("url", details.url, "initiator", details.initiator, > > "normalised", normalised); > > } > > > > and then browse to https://regex101.com/, you'll see this logged: > > > > url https://regex101.com/ initiator https://regex101.com normalised > > https://regex101.com/ > > > > Perhaps there's a more efficient approach that would have the same result, I'm > > not sure. I figured if it could happen with trailing slashes perhaps it could > > happen in other ways too (e.g. punycode) and so I did it this way. > > FWIW, this seems like a Chrome bug to me. All other URLs reported through the > API seem to be already normalized. OK I've filed an issue with that for them https://bugs.chromium.org/p/chromium/issues/detail?id=821042 https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode57 lib/csp.js:57: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) On 2018/03/07 17:27:07, Sebastian Noack wrote: > On 2018/03/07 15:07:01, kzar wrote: > > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > > If we check whether the document is whitelisted, after matching the filter, > > > wouldn't this cause wrong results in the devtools panel, i.e. showing an > > active > > > filter while it doesn't apply due to whitelisting? > > > > Well your comment made me realise I so far didn't log these CSP matches to the > > devtools panel. I've fixed that now and it's working OK for me. > > Forget about my original comment here. I mistakenly thought that filter matches > are implicitly logged. But good thing, we now thought about logging to the > devtools panel, at all. Acknowledged.
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). On 2018/03/12 16:33:42, kzar wrote: > On 2018/03/07 17:27:07, Sebastian Noack wrote: > > On 2018/03/07 15:07:01, kzar wrote: > > > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > > > Might this be the case when XHR (or fetch) is used from inside a > > > ServiceWorker? > > > > > > I'm not sure what's causing this so far, I know the behaviour doesn't happen > > in > > > Firefox. I filed a Chromium bug for this but had little help so far. > > > > Is there an example in which simply ignoring "xmlhttprequest" would give false > > negatives? My suspicion is that these requests are actual XHR/fetch requests, > > and that the tabId of -1 doesn't indicate a loading document, but that the > > origin is a ServiceWorker. > > Yes, the website I noticed this happening was http://regex101.com. Add a $csp filter > for that website and then try adjusting this listener to ignore xmlhttprequest > events. Ok, what's going on here is that the request you see with tabId = -1, frameId = -1, type = xmlhttprequest and initiator = https://regex.101.com, are requests sent using fetch() as part of the caching strategy implemented in the ServiceWorker. This is pretty standard usage of ServiceWorkers. And FWIW, this behavior is consistent with onBeforeRequest listeners. Moreover, the assumption that these request are only (top-level) documents is wrong. https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode66 lib/csp.js:66: hostname, cspMatch); On 2018/03/07 17:27:07, Sebastian Noack wrote: > This is redundant, checkWhitelisted() is already calling > logWhitelistedDocument(). What is about this comment? https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode100 lib/csp.js:100: if (devtools) On 2018/03/07 17:27:07, Sebastian Noack wrote: > Is this check still necessary? IIRC, require('devtools') only returned null on > Safari, where we don't include that module. Now, where we use WebPack it even > gets automatically included as soon as any other module requires it. What is about this comment?
Patch Set 5 : Rebased, ignored requests cached by ServiceWorkers and addressed Sebastian's feedback 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). On 2018/03/13 03:58:13, Sebastian Noack wrote: > On 2018/03/12 16:33:42, kzar wrote: > > On 2018/03/07 17:27:07, Sebastian Noack wrote: > > > On 2018/03/07 15:07:01, kzar wrote: > > > > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > > > > Might this be the case when XHR (or fetch) is used from inside a > > > > ServiceWorker? > > > > > > > > I'm not sure what's causing this so far, I know the behaviour doesn't > happen > > > in > > > > Firefox. I filed a Chromium bug for this but had little help so far. > > > > > > Is there an example in which simply ignoring "xmlhttprequest" would give > false > > > negatives? My suspicion is that these requests are actual XHR/fetch > requests, > > > and that the tabId of -1 doesn't indicate a loading document, but that the > > > origin is a ServiceWorker. > > > > Yes, the website I noticed this happening was http://regex101.com. Add a $csp > filter > > for that website and then try adjusting this listener to ignore xmlhttprequest > > events. > > Ok, what's going on here is that the request you see with tabId = -1, frameId = > -1, type = xmlhttprequest and initiator = https://regex.101.com, are requests > sent using fetch() as part of the caching strategy implemented in the > ServiceWorker. This is pretty standard usage of ServiceWorkers. And FWIW, this > behavior is consistent with onBeforeRequest listeners. Moreover, the assumption > that these request are only (top-level) documents is wrong. Acknowledged. https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode44 lib/csp.js:44: let specificOnly = false; On 2018/03/07 15:07:01, kzar wrote: > On 2018/03/06 19:43:24, Sebastian Noack wrote: > > Nit: This variable seems superfluous here. You can just inline the value false > > when calling matchesAny() here, and only define the variable below when you > need > > to check for its value. > > Strictly you're right, but I added it early in an attempt to make our intent > clearer with first checking for all matches and then later checking if > specificOnly should apply. If you don't mind too much I'd prefer to keep it this > way. Now that the logic is simplified this doesn't add much to the clarity of the code IMO. Done. https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode66 lib/csp.js:66: hostname, cspMatch); On 2018/03/07 17:27:07, Sebastian Noack wrote: > This is redundant, checkWhitelisted() is already calling > logWhitelistedDocument(). Done. https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode72 lib/csp.js:72: On 2018/03/07 17:27:07, Sebastian Noack wrote: > Nit: I think it reads slightly better without a blank line here. Wouldn't insist > though. Done. https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode100 lib/csp.js:100: if (devtools) On 2018/03/07 17:27:07, Sebastian Noack wrote: > Is this check still necessary? IIRC, require('devtools') only returned null on > Safari, where we don't include that module. Now, where we use WebPack it even > gets automatically included as soon as any other module requires it. Done.
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 use stringifyURL(new URL(...)) everywhere else when passing an URL to the matcher. This is mostly neccessary because domains in filter lists are unicode, but reported as punycode by the browser. In fact you even import stringiyURL, but don't use it anywhere. Do we still have any scripts that aren't bundled as modules? Otherwise, we could configure ESLint to detect this. https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode41 lib/csp.js:41: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) Are filters like @@||example.com$csp even supposed to work, i.e. meant to recursively disable CSP filters in that document and any sub-document? https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome#new... metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js This change seems unrelated. I first suspected it to come from rebasing, but this change is not included in the current "next" bookmark.
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 (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/13 22:28:30, Sebastian Noack wrote: > It seems, we use stringifyURL(new URL(...)) everywhere else when passing an URL to the matcher Done. > In fact you even import stringiyURL, but don't use it anywhere. Oh, that's surprising since I thought ESLint would catch that! WhitelistFilter was unused too. > Do we still have any scripts that aren't bundled as modules? Otherwise, we > could configure ESLint to detect this. Well from the top of my head there's the content scripts and stuff like ext and the polyfills. 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/13 22:28:30, Sebastian Noack wrote: > Are filters like mailto:@@||example.com$csp even supposed to work, i.e. meant to > recursively disable CSP filters in that document and any sub-document? Yea, the devtools interface adds a filter like that when you click to add an exception fo a $csp hit as well. https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome#new... metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js On 2018/03/13 22:28:31, Sebastian Noack wrote: > This change seems unrelated. I first suspected it to come from rebasing, but > this change is not included in the current "next" bookmark. This change is necessary since I updated the adblockplusui dependency to include my $csp related changes. By the time this lands hopefully the dependency will have already been updated and this change won't be necessary. Sorry I thought I had left a comment to explain already. (No I'm not happy about #6310, we should instead combine adblockplusui and adblockpluschrome instead of introducing another module system in adblockplusui which apparently doesn't even give us source maps for the bundled UI code in our builds...)
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, kzar wrote: > > In fact you even import stringiyURL, but don't use it anywhere. > > Oh, that's surprising since I thought ESLint would catch that! WhitelistFilter > was unused too. ESLint doesn't catch it because it considers the script to have global (as opposed to module) scope, and therefore it might be possible that any global variable that isn't used here might be used in another script. This can be configured, but not sure if it's worth to add additional boilerplate in every file therefore. However, if every script would be bundled as module (or at least doesn't define global variables to be used only by other scripts) we could configure the scope globally. > > Do we still have any scripts that aren't bundled as modules? Otherwise, we > > could configure ESLint to detect this. > > Well from the top of my head there's the content scripts and stuff like ext and > the polyfills. Right, though the content scripts should be modules, 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 10:46:43, kzar wrote: > On 2018/03/13 22:28:30, Sebastian Noack wrote: > > Are filters like mailto:@@||example.com$csp even supposed to work, i.e. meant > to > > recursively disable CSP filters in that document and any sub-document? > > Yea, the devtools interface adds a filter like that when you click to add an > exception fo a $csp hit as well. This should work regardless of checking for CSP, here. If there is a whitelist filter for a particular request, defaultMatcher.matchesAny() will always return the whitelist filter, not a blocking filter. However, by checking for CSP here, those filters are applied recursively, e.g. if the top level document matches a $csp whitelisting filter, no $csp filter is applied in any subdocument either. I'm not sure which semantics are preferable here. How does uBlock behave?
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, Sebastian Noack wrote: > On 2018/03/14 10:46:43, kzar wrote: > > > In fact you even import stringiyURL, but don't use it anywhere. > > > > Oh, that's surprising since I thought ESLint would catch that! WhitelistFilter > > was unused too. > > ESLint doesn't catch it because it considers the script to have global (as > opposed to module) scope, and therefore it might be possible that any global > variable that isn't used here might be used in another script. This can be > configured, but not sure if it's worth to add additional boilerplate in every > file therefore. However, if every script would be bundled as module (or at least > doesn't define global variables to be used only by other scripts) we could > configure the scope globally. OK > > > Do we still have any scripts that aren't bundled as modules? Otherwise, we > > > could configure ESLint to detect this. > > > > Well from the top of my head there's the content scripts and stuff like ext > and > > the polyfills. > > Right, though the content scripts should be modules, right? But remember the problem there was that we have some scripts which run at preload and some at postload. To avoid the modules used by both being duplicated we'll have to combine them, for now we worked around that by sticking them in `window` instead. 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 15:57:27, Sebastian Noack wrote: > On 2018/03/14 10:46:43, kzar wrote: > > On 2018/03/13 22:28:30, Sebastian Noack wrote: > > > Are filters like mailto:@@||example.com$csp even supposed to work, i.e. > meant > > to > > > recursively disable CSP filters in that document and any sub-document? > > > > Yea, the devtools interface adds a filter like that when you click to add an > > exception fo a $csp hit as well. > > This should work regardless of checking for CSP, here. If there is a whitelist > filter for a particular request, defaultMatcher.matchesAny() will always return > the whitelist filter, not a blocking filter. However, by checking for CSP here, > those filters are applied recursively, e.g. if the top level document matches a > $csp whitelisting filter, no $csp filter is applied in any subdocument either. > I'm not sure which semantics are preferable here. How does uBlock behave? I think we need to check for CSP here as well since that filter option isn't applied by default. But perhaps I misunderstand.
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, kzar wrote: > On 2018/03/14 15:57:27, Sebastian Noack wrote: > > On 2018/03/14 10:46:43, kzar wrote: > > > On 2018/03/13 22:28:30, Sebastian Noack wrote: > > > > Are filters like mailto:@@||example.com$csp even supposed to work, i.e. > > meant > > > to > > > > recursively disable CSP filters in that document and any sub-document? > > > > > > Yea, the devtools interface adds a filter like that when you click to add an > > > exception fo a $csp hit as well. > > > > This should work regardless of checking for CSP, here. If there is a whitelist > > filter for a particular request, defaultMatcher.matchesAny() will always > return > > the whitelist filter, not a blocking filter. However, by checking for CSP > here, > > those filters are applied recursively, e.g. if the top level document matches > a > > $csp whitelisting filter, no $csp filter is applied in any subdocument either. > > I'm not sure which semantics are preferable here. How does uBlock behave? > > I think we need to check for CSP here as well since that filter option isn't > applied by default. But perhaps I misunderstand. I don't understand how this is related. Let's say we navigate to https://example.com/, and there are following two filters: ||example.com$csp=connect-src http:; @@||example.com$csp Then defaultMatcher.matchesAny(...), above in line 35, will return the whitelisting filter. In fact, you only check whether any filter (not whether a blocking filter) is returned. But besides that, the only reason to use checkWhitelisted(..., ... | typeMap.CSP) here (or at least a side effect of it) is that it will cause filters like @@||example.com$csp to be applied redundantly, as checkWhiteliested() will return any whitelisting filter that applies to either this or any document up the frame hierarchy. I'm just wondering if these semantics are desired? I personally, don't have an opinion. But if we cannot find any argument for one way or another, perhaps we should just go with the same sementics that uBlock implemented. https://codereview.adblockplus.org/29680696/diff/29722569/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29722569/lib/csp.js#newcode33 lib/csp.js:33: let hostname = getDecodedHostname(url); This seems inconsistent with the semantics of request blocking, where the $domain option refers to the hostname of the parent document.
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: > However, by checking for CSP here, those filters are applied > recursively, e.g. if the top level document matches a $csp > whitelisting filter, no $csp filter is applied in any subdocument > either. I'm not sure which semantics are preferable here. How does > uBlock behave? OK I just did a test, I added these two filters: ||static.kzar.co.uk$csp=img-src none; @@||holly.cat$csp and then navigated to https://holly.cat/genericblockhide-test.html and https://static.kzar.co.uk/genericblockhide-test/. You're right that so far we apply the exception recursively, so the images are shown for the former. It turns out uBlock doesn't, so the images are blocked for both. > I'm just wondering if these semantics are desired? I personally, > don't have an opinion. But if we cannot find any argument for one > way or another, perhaps we should just go with the same sementics > that uBlock implemented. Well I suppose it might be useful for filter authors to be able to turn $csp blocking off for any iframes in a page. Especially since adverts are often wrapped inside iframes these days. Another thought is that uBlock doesn't care as much about whitelisting since it does not support AA, where as to us it's pretty important. On the other hand I'm generally keen for compatibility between extensions as to how filters are handled. I guess I can see arguments either way. > In fact, you only check whether any filter (not whether a blocking > filter) is returned. Yea, I did check to see if the filter was a WhitelistFilter previously and logged the whitelist hit manually[1], but you pointed out that wasn't necessary since checkWhitelisted would do the same thing anyway. I suppose I could alter this logic to search specifically for a BlacklistFilter, but would that change anything? I figure it wouldn't and if anything it would make things slightly slower if we continue searching for a BlacklistFilter when a WhitelistFilter would have been already returned. I'm open to suggestions anyway. [1] - https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode66 https://codereview.adblockplus.org/29680696/diff/29722569/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29722569/lib/csp.js#newcode33 lib/csp.js:33: let hostname = getDecodedHostname(url); On 2018/03/14 20:19:42, Sebastian Noack wrote: > This seems inconsistent with the semantics of request blocking, where the > $domain option refers to the hostname of the parent document. Well I suppose with request blocking we are generally blocking a request, instead of injecting something that will then block child requests (or similar). I'm not sure what else we can do here, do you have any suggestions?
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, kzar wrote: > Sebastian Noack wrote: > > > However, by checking for CSP here, those filters are applied > > recursively, e.g. if the top level document matches a $csp > > whitelisting filter, no $csp filter is applied in any subdocument > > either. I'm not sure which semantics are preferable here. How does > > uBlock behave? > > OK I just did a test, I added these two filters: > > ||static.kzar.co.uk$csp=img-src none; > mailto:@@||holly.cat$csp > > and then navigated to https://holly.cat/genericblockhide-test.html and > https://static.kzar.co.uk/genericblockhide-test/. You're right that so far > we apply the exception recursively, so the images are shown for the former. > It turns out uBlock doesn't, so the images are blocked for both. > > > I'm just wondering if these semantics are desired? I personally, > > don't have an opinion. But if we cannot find any argument for one > > way or another, perhaps we should just go with the same sementics > > that uBlock implemented. > > Well I suppose it might be useful for filter authors to be able to turn > $csp blocking off for any iframes in a page. Especially since adverts are > often wrapped inside iframes these days. Another thought is that uBlock > doesn't care as much about whitelisting since it does not support AA, > where as to us it's pretty important. On the other hand I'm generally keen > for compatibility between extensions as to how filters are handled. I guess > I can see arguments either way. Good point, as far as Acceptable Ads is concerned, I can think of a scenario where EasyList might want to inject a CSP in frames served by adserver.com, but if such a frame appears on a website whitelisted through Acceptable Ads, we might need to be able to prevent the CSP to be injected in that context. On the other hand, once you fixed the other issue, the same could be achieved with @@||adserver.com$csp,domain=aapartner.com. As for consistency, $subdocument exception rules aren't applied recursively either. However, $document and $elemhide exception rules are, but these options are special as they only work for whitelisting filters. If neither of us feels particular strong about one way or another, let's leave it up to Arthur. > > In fact, you only check whether any filter (not whether a blocking > > filter) is returned. > > Yea, I did check to see if the filter was a WhitelistFilter previously and > logged the whitelist hit manually[1], but you pointed out that wasn't > necessary since checkWhitelisted would do the same thing anyway. > > I suppose I could alter this logic to search specifically for a > BlacklistFilter, but would that change anything? I figure it wouldn't and > if anything it would make things slightly slower if we continue searching > for a BlacklistFilter when a WhitelistFilter would have been already > returned. I'm open to suggestions anyway. > > [1] - > https://codereview.adblockplus.org/29680696/diff/29716591/lib/csp.js#newcode66 If we give $csp exception rules recursive semantics, logWhitelistedDocument() is redundant with checkWhitelisted(). Otherwise, we should just call logRequest() with whatever (blocking or whitlising) filter matched. https://codereview.adblockplus.org/29680696/diff/29722569/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29722569/lib/csp.js#newcode33 lib/csp.js:33: let hostname = getDecodedHostname(url); On 2018/03/15 13:10:13, kzar wrote: > On 2018/03/14 20:19:42, Sebastian Noack wrote: > > This seems inconsistent with the semantics of request blocking, where the > > $domain option refers to the hostname of the parent document. > > Well I suppose with request blocking we are generally blocking a request, > instead of injecting something that will then block child requests (or similar). > I'm not sure what else we can do here, do you have any suggestions? I don't see how the difference between blocking requests vs injecting headers, calls for an inconsistency here. FWIW $popup filters have the same semantics. We have access to the parentFrameId, and can get the parent frame's URL, to make the behavior consistent here. So that ||foo.com$csp=...,domain=bar.com, only injects a CSP on foo.com if embedded in a frame on bar.com.
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): 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 17:14:24, Sebastian Noack wrote: > If neither of us feels particular strong about one way or another, > let's leave it up to Arthur. OK, I've sent him a message. https://codereview.adblockplus.org/29680696/diff/29722569/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29722569/lib/csp.js#newcode33 lib/csp.js:33: let hostname = getDecodedHostname(url); On 2018/03/15 17:14:25, Sebastian Noack wrote: > On 2018/03/15 13:10:13, kzar wrote: > > On 2018/03/14 20:19:42, Sebastian Noack wrote: > > > This seems inconsistent with the semantics of request blocking, where the > > > $domain option refers to the hostname of the parent document. > > > > Well I suppose with request blocking we are generally blocking a request, > > instead of injecting something that will then block child requests (or > similar). > > I'm not sure what else we can do here, do you have any suggestions? > > I don't see how the difference between blocking requests vs injecting headers, > calls for an inconsistency here. FWIW $popup filters have the same semantics. > > We have access to the parentFrameId, and can get the parent frame's URL, to make > the behavior consistent here. So that ||foo.com$csp=...,domain=bar.com, only > injects a CSP on http://foo.com if embedded in a frame on http://bar.com. Done, look OK?
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().
Patch Set 8 : Use extractHostFromFrame and attempt to match $csp exceptions non-recursively 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: > ... let's leave it up to Arthur. He voted for non-recursively. > If we give $csp exception rules recursive semantics, > logWhitelistedDocument() is redundant with checkWhitelisted(). > Otherwise, we should just call logRequest() with whatever > (blocking or whitlising) filter matched. I've attempted to do what you said with this latest patchset. Problem is it's not working. If I add these filters: $csp=script-src none;,domain=reddit.com @@||reddit.com/$csp and then browse to https://reddit.com I find that hits for both filters are listed in our developer tools pane and the Content Security Policy is injected. I find this stuff quite confusing, any idea what's going wrong? (My only guess is that I'm checking whitelisting with the wrong frame, which was previously masked by the fact by the fact that whitelisting was being applied recursively. I didn't realise that adding the CSP type to the bitmask had that effect, I had assumed it helped since filters aren't given the CSP filter type by default.) 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); On 2018/03/15 19:50:53, Sebastian Noack wrote: > Please use extractHostFromFrame(). Done.
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, kzar wrote: > > Sebastian Noack wrote: > > > ... let's leave it up to Arthur. > > He voted for non-recursively. > > > If we give $csp exception rules recursive semantics, > > logWhitelistedDocument() is redundant with checkWhitelisted(). > > Otherwise, we should just call logRequest() with whatever > > (blocking or whitlising) filter matched. > > I've attempted to do what you said with this latest patchset. Problem is it's > not working. If I add these filters: > > $csp=script-src http://none;,domain=reddit.com > @@||reddit.com/$csp > > and then browse to https://reddit.com I find that hits for both filters are > listed in our developer tools pane and the Content Security Policy is injected. > I find this stuff quite confusing, any idea what's going wrong? This behavior seems expected/correct to me. $csp=script-src http://none;,domain=reddit.com This filter is meant to inject the CSP in any subframe loaded on reddit.com. @@||reddit.com/$csp This filter is meant to prevent any CSP from being injected in documents loaded directly from reddit.com. So with those two filters, we essentially inject the CSP in all third-party frames on reddit.com. I suppose, you meant to test following two filters instead: ||reddit.com^$csp=script-src http://none; @@||reddit.com^$csp Which might not work either, since I cannot find where you handle $csp exception rules at all. (See other comment.) https://codereview.adblockplus.org/29680696/diff/29724564/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29724564/lib/csp.js#newcode42 lib/csp.js:42: hostname = getDecodedHostname(url); Is this micro-optimization worth it? Otherwise, this code could be simplified: let hostname = extractHostFromFrame(parentFrame) || getDecodedHostname(url); let thirdParty = isThirdParty(url, hostname); https://codereview.adblockplus.org/29680696/diff/29724564/lib/csp.js#newcode67 lib/csp.js:67: if (checkWhitelisted(page, frame)) Why not doing this check earlier? https://codereview.adblockplus.org/29680696/diff/29724564/lib/csp.js#newcode72 lib/csp.js:72: details.responseHeaders.push({ It seems this code path is reached if cspMatch is a whitelisting filter. https://codereview.adblockplus.org/29680696/diff/29724564/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29680696/diff/29724564/lib/requestBlocker.... lib/requestBlocker.js:71: // POPUP, CSP and ELEMHIDE filters aren't blocked on the request level but by Technically, the CSP filter is implemented on the request level. Perhaps, this comment would be more acurrate and less confusing with following change, now: "blocked on the request level" => "mapped to resource types"
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, frame, typeMap.DOCUMENT | typeMap.CSP)) On 2018/03/16 23:52:38, Sebastian Noack wrote: > On 2018/03/16 12:54:59, kzar wrote: > > > Sebastian Noack wrote: > > > > > ... let's leave it up to Arthur. > > > > He voted for non-recursively. > > > > > If we give $csp exception rules recursive semantics, > > > logWhitelistedDocument() is redundant with checkWhitelisted(). > > > Otherwise, we should just call logRequest() with whatever > > > (blocking or whitlising) filter matched. > > > > I've attempted to do what you said with this latest patchset. Problem is it's > > not working. If I add these filters: > > > > $csp=script-src http://none;,domain=reddit.com > > mailto:@@||reddit.com/$csp > > > > and then browse to https://reddit.com I find that hits for both filters are > > listed in our developer tools pane and the Content Security Policy is > injected. > > I find this stuff quite confusing, any idea what's going wrong? > > This behavior seems expected/correct to me. > > $csp=script-src http://none;,domain=reddit.com > > This filter is meant to inject the CSP in any subframe loaded on http://reddit.com. > > mailto:@@||reddRit.com/$csp > > This filter is meant to prevent any CSP from being injected in documents loaded > directly from http://reddit.com. > > So with those two filters, we essentially inject the CSP in all third-party > frames on http://reddit.com. > > I suppose, you meant to test following two filters instead: > > ||reddit.com^$csp=script-src http://none; > mailto:@@||reddit.com^$csp > > Which might not work either, since I cannot find where you handle $csp exception > rules at all. (See other comment.) Thanks for the explanation, Done. https://codereview.adblockplus.org/29680696/diff/29724564/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29724564/lib/csp.js#newcode42 lib/csp.js:42: hostname = getDecodedHostname(url); On 2018/03/16 23:52:38, Sebastian Noack wrote: > Is this micro-optimization worth it? Otherwise, this code could be simplified: > > let hostname = extractHostFromFrame(parentFrame) || getDecodedHostname(url); > let thirdParty = isThirdParty(url, hostname); Done. https://codereview.adblockplus.org/29680696/diff/29724564/lib/csp.js#newcode67 lib/csp.js:67: if (checkWhitelisted(page, frame)) On 2018/03/16 23:52:39, Sebastian Noack wrote: > Why not doing this check earlier? Done. https://codereview.adblockplus.org/29680696/diff/29724564/lib/csp.js#newcode72 lib/csp.js:72: details.responseHeaders.push({ On 2018/03/16 23:52:38, Sebastian Noack wrote: > It seems this code path is reached if cspMatch is a whitelisting filter. Done. https://codereview.adblockplus.org/29680696/diff/29724564/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29680696/diff/29724564/lib/requestBlocker.... lib/requestBlocker.js:71: // POPUP, CSP and ELEMHIDE filters aren't blocked on the request level but by On 2018/03/16 23:52:39, Sebastian Noack wrote: > Technically, the CSP filter is implemented on the request level. Perhaps, this > comment would be more acurrate and less confusing with following change, now: > > "blocked on the request level" => "mapped to resource types" Done.
https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome#new... metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js On 2018/03/14 10:46:43, kzar wrote: > On 2018/03/13 22:28:31, Sebastian Noack wrote: > > This change seems unrelated. I first suspected it to come from rebasing, but > > this change is not included in the current "next" bookmark. > > This change is necessary since I updated the adblockplusui dependency to include > my $csp related changes. By the time this lands hopefully the dependency will > have already been updated and this change won't be necessary. Sorry I thought I > had left a comment to explain already. There is now a separate issue for the adblockplusui depdency update (#6476). I marked it as blocking this change. https://codereview.adblockplus.org/29680696/diff/29727593/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29727593/lib/csp.js#newcode34 lib/csp.js:34: let parentFrame = details.parentFrameId != -1 && Nit: ext.getFrame(details.tabId, -1) just returns undefined, rendering this check redundant. https://codereview.adblockplus.org/29680696/diff/29727593/lib/csp.js#newcode53 lib/csp.js:53: let specificOnly = checkWhitelisted(page, frame, typeMap.GENERICBLOCK); checkWhitelisted() returns a filter object (or null). Both, in lib/requestBlocker.js and lib/popupBlocker.js, we turn it into a boolean (using !!), before passing the value to matchesAny(). https://codereview.adblockplus.org/29680696/diff/29727593/lib/csp.js#newcode68 lib/csp.js:68: FilterNotifier.emit("filter.hitCount", cspMatch, 0, 0, page); This seems inconsistent. For requests, we emit filter.hitCount also if a whitelisting filter matched. For reference, it seems that for popups we don't emit filter.hitCount at all, but IMO this is a bug.
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#new... metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js On 2018/03/19 22:25:11, Sebastian Noack wrote: > On 2018/03/14 10:46:43, kzar wrote: > > On 2018/03/13 22:28:31, Sebastian Noack wrote: > > > This change seems unrelated. I first suspected it to come from rebasing, but > > > this change is not included in the current "next" bookmark. > > > > This change is necessary since I updated the adblockplusui dependency to > include > > my $csp related changes. By the time this lands hopefully the dependency will > > have already been updated and this change won't be necessary. Sorry I thought > I > > had left a comment to explain already. > > There is now a separate issue for the adblockplusui depdency update (#6476). > I marked it as blocking this change. Acknowledged. When that lands I'll rebase this change on top. https://codereview.adblockplus.org/29680696/diff/29727593/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29727593/lib/csp.js#newcode34 lib/csp.js:34: let parentFrame = details.parentFrameId != -1 && On 2018/03/19 22:25:12, Sebastian Noack wrote: > Nit: ext.getFrame(details.tabId, -1) just returns undefined, rendering this > check redundant. Done. https://codereview.adblockplus.org/29680696/diff/29727593/lib/csp.js#newcode53 lib/csp.js:53: let specificOnly = checkWhitelisted(page, frame, typeMap.GENERICBLOCK); On 2018/03/19 22:25:12, Sebastian Noack wrote: > checkWhitelisted() returns a filter object (or null). > Both, in lib/requestBlocker.js and lib/popupBlocker.js, > we turn it into a boolean (using !!), before passing the value to matchesAny(). Done. https://codereview.adblockplus.org/29680696/diff/29727593/lib/csp.js#newcode68 lib/csp.js:68: FilterNotifier.emit("filter.hitCount", cspMatch, 0, 0, page); On 2018/03/19 22:25:12, Sebastian Noack wrote: > This seems inconsistent. For requests, we emit filter.hitCount also if a > whitelisting filter matched. For reference, it seems that for popups we don't > emit filter.hitCount at all, but IMO this is a bug. Done.
Looks good to me, once rebased on top of the adblockplusui dependency update, and the adblockpluscore dependency got updated as well.
Patch Set 11 : Rebased and updated adblockpluscore dependency (Still waiting for the adblockplusui dependency update however.)
Rebased
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
