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

lib/csp.js

Index: lib/csp.js
diff --git a/lib/csp.js b/lib/csp.js
index 6e7656d1b552a99a74279fefe5fe037a13a89840..0efde04f9acafef0b0a23fe600fdfa05c697964d 100644
--- a/lib/csp.js
+++ b/lib/csp.js
@@ -17,53 +17,79 @@
 
 "use strict";
 
-// The webRequest API doesn't support WebSocket connection blocking in Microsoft
-// Edge and versions of Chrome before 58. Therefore for those we inject CSP
-// headers below as a workaround. See https://crbug.com/129353 and
-// https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/10297376/
-if (!browser.webRequest.ResourceType ||
-    !("WEBSOCKET" in browser.webRequest.ResourceType))
+const {defaultMatcher} = require("matcher");
+const {BlockingFilter, RegExpFilter} = require("filterClasses");
+const {getDecodedHostname, stringifyURL} = require("url");
+const {checkWhitelisted} = require("whitelisting");
+const {FilterNotifier} = require("filterNotifier");
+
+const {typeMap} = RegExpFilter;
+
+browser.webRequest.onHeadersReceived.addListener(details =>
 {
-  const {defaultMatcher} = require("matcher");
-  const {BlockingFilter, RegExpFilter} = require("filterClasses");
-  const {getDecodedHostname} = require("url");
-  const {checkWhitelisted} = require("whitelisting");
+  // 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

+  {
+    return;
+  }
 
-  browser.webRequest.onHeadersReceived.addListener(details =>
+  // 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.

+
+  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)
   {
-    let hostname = getDecodedHostname(new URL(details.url));
-    let match = defaultMatcher.matchesAny("", RegExpFilter.typeMap.WEBSOCKET,
-                                          hostname, false, null, true);
-    if (match instanceof BlockingFilter &&
-        !checkWhitelisted(new ext.Page({id: details.tabId}),
-                          ext.getFrame(details.tabId, details.frameId)))
+    if (details.tabId > -1)
     {
-      details.responseHeaders.push({
-        name: "Content-Security-Policy",
-        // We're blocking WebSockets here by adding a connect-src restriction
-        // since the Chrome extension API does not allow us to intercept them.
-        // https://bugs.chromium.org/p/chromium/issues/detail?id=129353
-        //
-        // We also need the frame-src and object-src restrictions since CSPs
-        // are not inherited from the parent for documents with data: and blob:
-        // URLs, see https://crbug.com/513860.
-        //
-        // We must use the deprecated child-src directive instead of worker-src
-        // since that's not supported yet (as of Chrome 56.)
-        //
-        // "http:" also includes "https:" implictly.
-        // https://www.chromestatus.com/feature/6653486812889088
-        value: "connect-src http:; child-src http:; " +
-               "frame-src http:; object-src http:"
-      });
-      return {responseHeaders: details.responseHeaders};
+      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/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;
+      }
+
+      // 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))
+    {
+      return;
     }

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

Nit: Don't we omit braces anymore if there is only one line in the block?

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

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.

-  }, {
-    urls: ["http://*/*", "https://*/*"],
-    // We must also intercept script requests since otherwise Web Workers can
-    // be abused to execute scripts for which our Content Security Policy
-    // won't be injected.
-    // https://github.com/gorhill/uBO-Extra/issues/19
-    types: ["main_frame", "sub_frame", "script"]
-  }, ["blocking", "responseHeaders"]);
-}
+
+    details.responseHeaders.push({
+      name: "Content-Security-Policy",
+      value: cspMatch.csp
+    });
+
+    return {responseHeaders: details.responseHeaders};
+  }
+}, {
+  urls: ["http://*/*", "https://*/*"],
+  types: ["main_frame", "sub_frame", "xmlhttprequest"]
+}, ["blocking", "responseHeaders"]);