Issue 6544 - Prevent requests sent by Chrome or Adblock Plus from being blocked

lib/requestBlocker.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/>.
  */
 
 /** @module requestBlocker */
 
 "use strict";
 
-const {Filter, RegExpFilter, BlockingFilter} = require("filterClasses");
-const {Subscription} = require("subscriptionClasses");
-const {defaultMatcher} = require("matcher");
-const {FilterNotifier} = require("filterNotifier");
-const {Prefs} = require("prefs");
-const {checkWhitelisted, getKey} = require("whitelisting");
-const {stringifyURL, extractHostFromFrame, isThirdParty} = require("url");
-const {port} = require("messaging");
-const devtools = require("devtools");
+const {Filter, RegExpFilter, BlockingFilter} =
+  require("../adblockpluscore/lib/filterClasses");
+const {Subscription} = require("../adblockpluscore/lib/subscriptionClasses");
+const {defaultMatcher} = require("../adblockpluscore/lib/matcher");
+const {FilterNotifier} = require("../adblockpluscore/lib/filterNotifier");
+const {Prefs} = require("./prefs");
+const {checkWhitelisted, getKey} = require("./whitelisting");
+const {stringifyURL, extractHostFromFrame, isThirdParty} = require("./url");
+const {port} = require("./messaging");
+const devtools = require("./devtools");
 
 const extensionProtocol = new URL(browser.extension.getURL("")).protocol;
-const ignoredOrigins = new Set([extensionProtocol, "chrome:"]);
 
 // Chrome can't distinguish between OBJECT_SUBREQUEST and OBJECT requests.
 if (!browser.webRequest.ResourceType ||
     !("OBJECT_SUBREQUEST" in browser.webRequest.ResourceType))
 {
   RegExpFilter.typeMap.OBJECT_SUBREQUEST = RegExpFilter.typeMap.OBJECT;
 }
 
 // Map of content types reported by the browser to the respecitve content types
 // used by Adblock Plus. Other content types are simply mapped to OTHER.
@@ skipping 96 matching lines @@
   // Ignore requests sent by extensions or by the browser itself:
   // * Firefox intercepts requests sent by any extensions, indicated with
   //   an "originURL" starting with "moz-extension:".
   // * Chromium intercepts requests sent by this extension only, indicated
   //   on Chromium >=63 with an "initiator" starting with "chrome-extension:".
   // * On Firefox, requests that don't relate to any document or extension are
   //   indicated with an "originUrl" starting with "chrome:".
   // * On Chromium >=63, requests that don't relate to any document or extension
   //   have no "initiator". But since on older Chromium versions, no request
   //   has an "initiator", we have to check for the tabId as well.
-  if (originUrl ? ignoredOrigins.has(originUrl.protocol) : details.tabId == -1)

[kzar, 2018/04/06 14:48:10]

Or perhaps like this?

if (originUrl && ignoredOrigins.has(originUrl.protocol) || details.tabId == -1)
  return;

Or even better IMO (so you can comment each part separately)

if (originUrl && ignoredOrigins.has(originUrl.protocol))
  return;

if (details.tabId == -1)
  return;

[Sebastian Noack, 2018/04/06 17:55:46]

These checks aren't equivalent. We only want to bail if we neither have a valid
tabId nor an originUrl. Otherwise requests sent by Service Workers would be
ignored too.

[kzar, 2018/04/09 11:08:44]

Ah right I see. Thing is I think this demonstrates how this nested conditional
is kind of confusing. I know it doesn't look as pretty but I think it's better
to do it with if... else..., it's way more obvious that way.

if (originURL)
{
  if (ignoredOrigins.has(originURL.protocol))
    return;
}
else if (details.tabId == -1)
  return;

[Sebastian Noack, 2018/04/09 21:37:16]

I don't see how the ternary operation is any more complicated to parse (for
humans) than binary logical operations. But fine, here you go.

+  if (originUrl)
+  {
+    if (originUrl.protocol == extensionProtocol ||
+        originUrl.protocol == "chrome:")
+      return;
+  }
+  else if (details.tabId == -1)
     return;
 
   let page = null;
   let frame = null;
   if (details.tabId != -1)
   {
     page = new ext.Page({id: details.tabId});
     frame = ext.getFrame(
       details.tabId,
       // We are looking for the frame that contains the element which
@@ skipping 107 matching lines @@
 
 port.on("request.blockedByRTCWrapper", (msg, sender) =>
 {
   return ext.webRequest.onBeforeRequest._dispatch(
      new URL(msg.url),
      "webrtc",
      sender.page,
      sender.frame
   ).includes(false);
 });