[$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} = 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 =>
 {
-  // Chrome seems to sometimes give the response type of "xmlhttprequest"
-  // instead of "main_frame", but when it does the tabId is always -1 and
-  // the URL matches the initiator's URL (once normalised).

[Sebastian Noack, 2018/03/06 19:43:24]

Might this be the case when XHR (or fetch) is used from inside a ServiceWorker?

[kzar, 2018/03/07 15:07:01]

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.

[Sebastian Noack, 2018/03/07 17:27:07]

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.

[kzar, 2018/03/12 16:33:42]

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.

[Sebastian Noack, 2018/03/13 03:58:13]

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. 

[kzar, 2018/03/13 18:11:01]

Acknowledged.

-  // https://bugs.chromium.org/p/chromium/issues/detail?id=805649
-  if (details.type == "xmlhttprequest" &&
-      (details.tabId > -1 || details.initiator &&
-       details.url != new URL(details.initiator).toString()))

[Sebastian Noack, 2018/03/06 19:43:24]

Why is it necessary to normalize the URL? Shouldn't all URLs already be
normalized when reported through the extension API?

[kzar, 2018/03/07 15:07:01]

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.

[Sebastian Noack, 2018/03/07 17:27:07]

FWIW, this seems like a Chrome bug to me. All other URLs reported through the
API seem to be already normalized.

[kzar, 2018/03/12 16:33:42]

OK I've filed an issue with that for them
https://bugs.chromium.org/p/chromium/issues/detail?id=821042

+  let url = new URL(details.url);
+  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)
   {
-    return;
-  }
+    let page = new ext.Page({id: details.tabId, url: details.url});
+    let frame = ext.getFrame(details.tabId, details.frameId);
 
-  // To avoid an extra matchesAny for the common case let's assume no
-  // $genericblock filters apply when searching for a matching $csp filter,
-  // we can double check later if necessary.
-  let specificOnly = false;

[Sebastian Noack, 2018/03/06 19:43:24]

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.

[kzar, 2018/03/07 15:07:01]

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.

[kzar, 2018/03/13 18:11:01]

Now that the logic is simplified this doesn't add much to the clarity of the
code IMO. Done.

+    if (checkWhitelisted(page, frame))
+      return;
 
-  let url = new URL(details.url);
-  let hostname = getDecodedHostname(url);
-  let cspMatch = defaultMatcher.matchesAny(details.url, typeMap.CSP, hostname,
-                                           false, null, specificOnly);
-  if (cspMatch instanceof BlockingFilter)
-  {
-    if (details.tabId > -1)
+    // 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);
+    if (specificOnly)
     {
-      let page = new ext.Page({id: details.tabId, url: details.url});
-      let frame = ext.getFrame(details.tabId, details.frameId);
+      cspMatch = defaultMatcher.matchesAny(urlString, typeMap.CSP, hostname,
+                                           thirdParty, null, specificOnly);
+      if (!cspMatch)
+        return;
+    }
 
-      if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP))

[Sebastian Noack, 2018/03/06 19:43:24]

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?

[kzar, 2018/03/07 15:07:01]

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.

[Sebastian Noack, 2018/03/07 17:27:07]

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.

[kzar, 2018/03/12 16:33:42]

Acknowledged.

-      {
-        return;
-      }
+    devtools.logRequest(page, urlString, "CSP", hostname, thirdParty, null,
+                        specificOnly, cspMatch);
+    FilterNotifier.emit("filter.hitCount", cspMatch, 0, 0, page);
 
-      // We pay the price now for skipping the specificOnly check earlier, we
-      // must check again for a CSP filter, this time making sure it's specific.
-      specificOnly = !!checkWhitelisted(page, frame, typeMap.GENERICBLOCK);
-      if (specificOnly)
-      {
-        cspMatch = defaultMatcher.matchesAny(details.url, typeMap.CSP, hostname,
-                                             false, null, specificOnly);
-        if (!(cspMatch instanceof BlockingFilter))
-          return;
-      }
-
-      FilterNotifier.emit("filter.hitCount", cspMatch, 0, 0, page);
-    }
-    // If we don't know the associated tab we'll have to check that if the URL
-    // is whitelisted manually. Things like $sitekey whitelisting and filter hit
-    // logging won't work, but it's the best we can do.
-    else if (defaultMatcher.whitelist.matchesAny(stringifyURL(url),
-                                                 typeMap.DOCUMENT | typeMap.CSP,
-                                                 hostname))
-    {
+    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", "xmlhttprequest"]
+  types: ["main_frame", "sub_frame"]
 }, ["blocking", "responseHeaders"]);