[$csp4 adblockpluschrome] Issue 5241 - Add support for Content Security Policy filters

lib/csp.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 const {defaultMatcher} = require("matcher");
-const {BlockingFilter,
-       RegExpFilter,
-       WhitelistFilter} = require("filterClasses");
-const {getDecodedHostname, stringifyURL} = require("url");
+const {RegExpFilter, WhitelistFilter} = require("filterClasses");
+const {extractHostFromFrame, getDecodedHostname,
+       isThirdParty, stringifyURL} = require("url");
 const {checkWhitelisted} = require("whitelisting");
 const {FilterNotifier} = require("filterNotifier");
 const devtools = require("devtools");
 
 const {typeMap} = RegExpFilter;
 
 browser.webRequest.onHeadersReceived.addListener(details =>
 {
   let url = new URL(details.url);
-  let hostname = getDecodedHostname(url);
-  let cspMatch = defaultMatcher.matchesAny(details.url, typeMap.CSP, hostname,

[Sebastian Noack, 2018/03/13 22:28:30]

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.

[kzar, 2018/03/14 10:46:43]

Done.
Oh, that's surprising since I thought ESLint would catch that! WhitelistFilter
was unused too.
Well from the top of my head there's the content scripts and stuff like ext and
the polyfills.

[Sebastian Noack, 2018/03/14 15:57:27]

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.
Right, though the content scripts should be modules, right?

[kzar, 2018/03/14 17:16:07]

OK
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.

-                                           false, null, false);
+  let urlString = stringifyURL(url);
+  let parentFrame = ext.getFrame(details.tabId, details.parentFrameId);
+  let hostname = extractHostFromFrame(parentFrame) || getDecodedHostname(url);
+  let thirdParty = isThirdParty(url, hostname);
+
+  let cspMatch = defaultMatcher.matchesAny(urlString, typeMap.CSP, hostname,
+                                           thirdParty, null, false);
   if (cspMatch)
   {
     let page = new ext.Page({id: details.tabId, url: details.url});
     let frame = ext.getFrame(details.tabId, details.frameId);
-    if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP))

[Sebastian Noack, 2018/03/13 22:28:30]

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?

[kzar, 2018/03/14 10:46:43]

Yea, the devtools interface adds a filter like that when you click to add an
exception fo a $csp hit as well.

[Sebastian Noack, 2018/03/14 15:57:27]

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?

[kzar, 2018/03/14 17:16:07]

I think we need to check for CSP here as well since that filter option isn't
applied by default. But perhaps I misunderstand.

[Sebastian Noack, 2018/03/14 20:19:42]

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.

[kzar, 2018/03/15 13:10:13]

Sebastian Noack wrote:
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.
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.
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

[Sebastian Noack, 2018/03/15 17:14:24]

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.
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.

[kzar, 2018/03/15 18:18:43]

OK, I've sent him a message.

[kzar, 2018/03/16 12:54:59]

He voted for non-recursively.
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.)

[Sebastian Noack, 2018/03/16 23:52:38]

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.)

[kzar, 2018/03/19 14:41:30]

Thanks for the explanation, Done.

+
+    if (checkWhitelisted(page, frame))
       return;
 
     // To avoid an extra matchesAny for the common case we assumed no
     // $genericblock filters applied when searching for a matching $csp filter.
     // We must now pay the price by first checking for a $genericblock filter
     // and if necessary that our $csp filter is specific.
-    let specificOnly = checkWhitelisted(page, frame, typeMap.GENERICBLOCK);
+    let specificOnly = !!checkWhitelisted(page, frame, typeMap.GENERICBLOCK);
     if (specificOnly)
     {
-      cspMatch = defaultMatcher.matchesAny(details.url, typeMap.CSP, hostname,
-                                           false, null, specificOnly);
-      if (!(cspMatch instanceof BlockingFilter))
+      cspMatch = defaultMatcher.matchesAny(urlString, typeMap.CSP, hostname,
+                                           thirdParty, null, specificOnly);
+      if (!cspMatch)
         return;
     }
 
+    devtools.logRequest(page, urlString, "CSP", hostname, thirdParty, null,
+                        specificOnly, cspMatch);
     FilterNotifier.emit("filter.hitCount", cspMatch, 0, 0, page);
-    devtools.logRequest(page, details.url, "CSP", hostname, false, null,
-                        specificOnly, cspMatch);
+
+    if (cspMatch instanceof WhitelistFilter)
+      return;
 
     details.responseHeaders.push({
       name: "Content-Security-Policy",
       value: cspMatch.csp
     });
 
     return {responseHeaders: details.responseHeaders};
   }
 }, {
   urls: ["http://*/*", "https://*/*"],
   types: ["main_frame", "sub_frame"]
 }, ["blocking", "responseHeaders"]);